Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MYST-268 Add endpoint and CLI command for current IP #118

Merged
merged 9 commits into from Jan 26, 2018
Merged

MYST-268 Add endpoint and CLI command for current IP #118

merged 9 commits into from Jan 26, 2018

Conversation

donce
Copy link
Contributor

@donce donce commented Jan 24, 2018

No description provided.

Waldz
Waldz requested changes Jan 24, 2018
View reviewed changes
tequilapi/client/client.go Outdated
@@ -110,6 +110,20 @@ func (client *Client) Status() (status StatusDto, err error) {
return status, err
}

func (client *Client) Ip() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetIP()

tequilapi/client/client.go Outdated
defer response.Body.Close()

var ipData struct {
Ip string `json:"ip"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IP

tequilapi/endpoints/current_ip.go Outdated
)

type currentIPEndpoint struct {
ipResolver ipResolver
Copy link
Member

@Waldz Waldz Jan 24, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All ipifyClient could be this dependency. And finally it could be renamed ipifyClient -> IPResolver

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure I understood the suggestion - are you suggesting to refactor ipify.Client interface with all its implementations into functions (instead of a classes), i.e.:
func ipify.NewClient (ipify.Client, error) -> func ipify.NewIPResolver (IPResolver, error) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I suggest to inject full ipifyClient here ipResolver ipify.Client

tadovas
tadovas reviewed Jan 25, 2018
View reviewed changes
tequilapi/http_routes.go Outdated
func NewAPIRouter() *httprouter.Router {
router := httprouter.New()
router.HandleMethodNotAllowed = true

router.GET("/healthcheck", endpoints.HealthCheckEndpointFactory(time.Now, os.Getpid).HealthCheck)

ipifyClient := ipify.NewClient()
router.GET("/ip", endpoints.NewCurrentIPEndpoint(ipifyClient.GetPublicIP).CurrentIP)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can make a more generic endpoint and call it /network which can return more network information, not only ip?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not hard to rename later on if we have to add more fields - for now, all it does is return IP, because that's the user story.

Waldz
Waldz requested changes Jan 25, 2018
View reviewed changes
tequilapi/http_routes.go Outdated
func NewAPIRouter() *httprouter.Router {
router := httprouter.New()
router.HandleMethodNotAllowed = true

router.GET("/healthcheck", endpoints.HealthCheckEndpointFactory(time.Now, os.Getpid).HealthCheck)

ipifyClient := ipify.NewClient()
router.GET("/ip", endpoints.NewCurrentIPEndpoint(ipifyClient.GetPublicIP).CurrentIP)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should go somewhere under /v1..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe GET /v1/connection/ip is right place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have /v1, but it makes sense to move it inside connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to connection

tequilapi/http_routes.go Outdated
func NewAPIRouter() *httprouter.Router {
router := httprouter.New()
router.HandleMethodNotAllowed = true

router.GET("/healthcheck", endpoints.HealthCheckEndpointFactory(time.Now, os.Getpid).HealthCheck)

ipifyClient := ipify.NewClient()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont factory on the fly, pass dependency from upper layer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I just noticed that NewAPIRouter() factory is not right place to connect features

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to connection endpoints

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, added dependency injection.

@donce donce force-pushed the feature/MYST-268-current-ip-endpoint branch from 476781d to 82545d1 Compare January 25, 2018 09:31
@donce
Copy link
Contributor Author

donce commented Jan 25, 2018

@Waldz fixed all issues, ready for review :)

Waldz
Waldz previously approved these changes Jan 25, 2018
View reviewed changes
ip/rest_resolver.go Outdated
const IPIFY_API_CLIENT = "goclient-v0.1"
const IPIFY_API_LOG_PREFIX = "[ipify.api] "
const IpifyAPIURL = "https://api.ipify.org/"
const IpifiAPICLient = "goclient-v0.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ipifi -> Ipify..

  • Could be private constants

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

ip/resolver.go

type Client interface {
type Resolver interface {
GetPublicIP() (string, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be 2 implementations with same ip. Resolver interface e.g. NewPublicResolver(), NewOutboundResolver()

@donce donce force-pushed the feature/MYST-268-current-ip-endpoint branch from 8fc707d to 738377e Compare January 25, 2018 15:44
@donce donce force-pushed the feature/MYST-268-current-ip-endpoint branch from 738377e to 1552e06 Compare January 25, 2018 15:45
@donce donce merged commit 49a08fc into master Jan 26, 2018
@donce donce deleted the feature/MYST-268-current-ip-endpoint branch January 26, 2018 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tadovas tadovas tadovas left review comments

@Waldz Waldz Waldz approved these changes

@ignasbernotas ignasbernotas Awaiting requested review from ignasbernotas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@donce @tadovas @Waldz